Refactor RateLimitChecker #5521
Conversation
|
This is a WIP/discussion PR for now, I'll post comments inline. |
| false | ||
| end | ||
| check_method = "check_#{action}_limit" | ||
| result = respond_to?(check_method, true) ? send(check_method) : false |
There was a problem hiding this comment.
Note the true as second argument to respond_to? so it also considers private methods.
There was a problem hiding this comment.
TIL that respond_to? takes a second parameter!
| field :rate_limit_comment_creation, type: :integer, default: 9 | ||
| field :rate_limit_published_article_creation, type: :integer, default: 9 | ||
| field :rate_limit_image_upload, type: :integer, default: 9 |
There was a problem hiding this comment.
Don't forget to submit another PR after this to expose these rate limits to /internal/config
There was a problem hiding this comment.
I was torn on whether or not to do it in the same PR. In the end my reasoning was that I can finish the refactoring first, since the current version is also non-configurable. Then add the configuration options as separate PR, which has the advantage that the PRs are more limited in scope and therefore easier to review. WDYT?
| create_list(:comment, 10, user_id: user.id, commentable_id: article.id) | ||
| expect(described_class.new(user).limit_by_action("comment_creation")).to eq(true) | ||
| count = SiteConfig.rate_limit_comment_creation + 1 | ||
| create_list(:comment, count, user_id: user.id, commentable_id: article.id) |
There was a problem hiding this comment.
I haven't tested but would it be possible to mock or lower the rate limit as a precondition? Creating 10 comments in the DB when we can create just 2 or 3 it's a waste :D
There was a problem hiding this comment.
Good call, reduced the number of objects created.
|
@rhymes Thanks for the review. I also noted that we're still missing one |
pr-triage
bot
added
PR: reviewed-approved
| false | ||
| end | ||
| check_method = "check_#{action}_limit" | ||
| result = respond_to?(check_method, true) ? send(check_method) : false |
There was a problem hiding this comment.
TIL that respond_to? takes a second parameter!
pr-triage
bot
removed
the
PR: reviewed-approved
What type of PR is this? (check all applicable)
Description
The current rate limit checker uses hard-coded limits for everything, and the intention of this PR is to make it externally configurable. Additionally the code will be cleaned up a bit.
Related Tickets & Documents
#5520
Added to documentation?